Replace Paver quality and js_test commands #35159

salman2013 · 2024-07-23T08:55:57Z

Paver is deprecated, and we aim to fully remove it soon. You can find more details here:

Paver deprecation issue
Paver removal PR

A few Paver commands remain in the edx-platform, and to proceed with its removal, we've replaced the following Paver commands with equivalent Make commands:

Replaced paver run_pep8 with pycodestyle
Replaced paver run_eslint
Replaced paver run_stylelint
Replaced paver run_xsslint
Replaced paver run_pii_check
Replaced paver check_keywords
Replaced paver test_js* and paver diff_coverage

How to Test This PR:

In the edx-platform directory, you can use the following terminal commands to check for linting issues in Python and JavaScript files. Make sure your virtual environment is activated before running these commands.

make pycodestyle
make eslint
make stylelint
make xsslint
make pi_check
make check_keyword
make test-js
make test-coverage
make quality-test

For more details, please refer to the ticket: Replace paver quality and js_test commands

salman2013 · 2024-07-23T11:30:30Z

@feanil @kdmccormick
Should we remove the paver tests as well like https://github.com/openedx/edx-platform/tree/master/pavelib/paver_tests?

kdmccormick · 2024-07-23T14:10:18Z

@salman2013 as you remove Paver commands, please remove the tests that correspond to those commands.

kdmccormick · 2024-07-24T14:09:15Z

Thanks for the PR @salman2013, but this only goes part of the way. The goal of #35159 is not only to get rid of the paver ... commands. The goal is get rid of all the Python code that runs our quality checks. In the end, we should be able to delete the whole pavelib directory.

Keep in mind that there are several things that pavelib code did that are now completely unnecessary, for example:

junit.xml -- I don't know if we need to write this file any more. If nothing is obviously using it, it can be deleted.
violation counts -- There was a time when all checks had "violation limits", so we would need to keep track of the violation count, and then fail only if the count exceeded the limit. For many checks, the violation limit is now zero. If the violation limit for a check is zero, then that check can be simplified, since any violation is enough to fail it.

I recommend starting with one simple check, such as pycodestyle, and see if you can get that running without any Python scripts involved.

…er-commands

salman2013 · 2024-08-27T07:18:43Z

@kdmccormick I believe this PR is ready for another pass. Please take a look thanks.

kdmccormick

Most of this feedback is straightforward, except the very last item regarding subprocess.run. I'm happy to go over that in our sync tomorrow morning if my comment isn't clear.

Makefile

pavelib/utils/envs.py

Makefile

scripts/quality_test/quality_test.py

salman2013 · 2024-09-12T10:47:04Z

@kdmccormick Thanks for reviewing the PR, I have fixed all your comments, and the PR is ready for another pass.

kdmccormick · 2024-10-24T16:52:14Z

Taking another look...

kdmccormick

Here's a partial review-- I'll have some more comments tomorrow, and I still need to run each check locally. But generally this is looking good and it's exciting to see how much you were able to delete.

kdmccormick · 2024-10-24T20:49:28Z

Makefile

@@ -204,3 +204,29 @@ migrate: migrate-lms migrate-cms
 # Part of https://github.com/openedx/wg-developer-experience/issues/136
 ubuntu-requirements: ## Install ubuntu 22.04 system packages needed for `pip install` to work on ubuntu.
 	sudo apt install libmysqlclient-dev libxmlsec1-dev
+
+eslint:	## check javascript for quality issues


The standard interface from our other frontend repos is to add the linting & testing scripts to package.json, such that users execute them with npm run ....

Instead of adding Makefile targets for frontend scripts, could you add them to package.json? To be consistent with other repos, use:

npm run test (for test-js)

npm run coverage (for coverage-js)

npm run lint (for eslint, stylelint, and xsslint)

kdmccormick · 2024-10-24T20:51:07Z

Makefile

+coverage-js: ## run javascript coverage test
+	python scripts/js_test.py --option coverage
+
+quality: pycodestyle eslint stylelint xsslint pii_check check_keywords


I would rather have make quality either completely check quality (including mypy and pylint), or leave it out of the Makefile. Either is OK, up to you.

kdmccormick · 2024-10-25T18:53:16Z

I've created a series of PRs based on this branch to make sure that each check fails when a violation is added:

pycodestyle
- Validate pycodestyle kdmccormick/edx-platform#21
pii_check
- Validate pii_check kdmccormick/edx-platform#18
check_keywords
- Validate check_keywords kdmccormick/edx-platform#19
test-js
- Validate test-js kdmccormick/edx-platform#20
coverage-js
- Validate coverage-js kdmccormick/edx-platform#22
eslint
- Validate eslint kdmccormick/edx-platform#24
stylelint
- Validate stylelint kdmccormick/edx-platform#23
xsslint
- Validate xsslint kdmccormick/edx-platform#25

kdmccormick · 2024-10-28T14:37:33Z

@salman2013 I'm currently validating each individual check with this spreadsheet: https://docs.google.com/spreadsheets/d/1KHl4bD9z1OodgLsJLUkobL8R0itMikEOSbHmkIFsMok/edit?gid=0#gid=0

kdmccormick

pycodestyle and test-js are failing correctly 😄 Good job on those ✔️
pii_check has problems on this PR, but it also has problems on master-- see my comment below.
xsslint on this PR is failing to catch violations, although it works on master.
check_keywords on this PR is properly finding violations, but it does not print the message to CI output.
I'm still trying out coverage-js, eslint, stylelint-- I'll let you know how testing those goes.

kdmccormick · 2024-10-28T18:40:58Z

scripts/quality_test.py

+def run_pii_check():
+    """
+    Guarantee that all Django models are PII-annotated.
+    """
+    REPO_ROOT = repo_root()
+    REPORT_DIR = REPO_ROOT / 'reports'
+    pii_report_name = 'pii'
+    default_report_dir = (REPORT_DIR / pii_report_name)
+    report_dir = default_report_dir
+    output_file = os.path.join(report_dir, 'pii_check_{}.report')
+    env_report = []
+    pii_check_passed = True
+
+    for env_name, env_settings_file in (("CMS", "cms.envs.test"), ("LMS", "lms.envs.test")):
+        try:
+            print(f"Running {env_name} PII Annotation check and report")
+            print("-" * 45)
+
+            run_output_file = str(output_file).format(env_name.lower())
+            os.makedirs(report_dir, exist_ok=True)
+
+            # Prepare the environment for the command
+            env = {
+                **os.environ,  # Include the current environment variables
+                "DJANGO_SETTINGS_MODULE": env_settings_file  # Set DJANGO_SETTINGS_MODULE for each environment
+            }
+
+            command = [
+                "code_annotations",
+                "django_find_annotations",
+                "--config_file", ".pii_annotations.yml",
+                "--report_path", str(report_dir),
+                "--app_name", env_name.lower()
+            ]
+
+            # Run the command without shell=True
+            with open(run_output_file, 'w') as report_file:
+                subprocess.run(
+                    command,
+                    env=env,  # Pass the environment with DJANGO_SETTINGS_MODULE
+                    check=True,
+                    stdout=report_file,
+                    stderr=subprocess.STDOUT,
+                    text=True
+                )
+
+            # Extract results
+            uncovered_model_count, pii_check_passed_env, full_log = _extract_missing_pii_annotations(run_output_file)
+            env_report.append((
+                uncovered_model_count,
+                full_log,
+            ))
+
+        except BuildFailure as error_message:
+            fail_quality(pii_report_name, f'FAILURE: {error_message}')
+
+        # Update pii_check_passed based on the result of the current environment
+        if not pii_check_passed_env:
+            pii_check_passed = False
+
+    # If the PII check failed in any environment, fail the task
+    if not pii_check_passed:
+        fail_quality('pii', full_log)
+    else:
+        print("Successfully ran pii_check")


This function was giving a false-positive. That is, the check was passing even when there were PII errors. This is because the code_annotations shell command is missing the arguments --lint- and --coverage, which tell it to raise an error if PII linting or coverage fails.

Unfortunately, the exiting Paver PII check also raises false-positives! Both the Paver code and your new code write annotation checks to a file, and the use regex to read the result out of that file and determine whether to pass the check. However, if the report-generation command itself fails, then the check will pass. This has been happening for several years, during which time we've been building up a pile of PII annotation violations.

So:

We have PII violations on master. Brian Mesick will be looking into the possibility of fixing some of those. For this PR though, please lower the PII annotation threshold down from 94.5 to 71.6 so that it can pass on the existing codebase. You will also need to remove celery_utils.FailedTask from the .annotation_safe_list. When you are done with that, you should be able to run make pii_check locally and see that it passes.

Whenever possible, we really ought to stop this dance of writing reports to files and then parsing the results with regex. It is complicated and mistake-prone. Instead, we should just run the command, let its output flow out to the user, and then use only the command's exit code (via CalledProcessError) in order to determine whether to fail the CI check or not. ~~Below is my suggestion of how to do that for the pii_check function.~~ EDIT: Better yet, please replace the Python implementation of run_pii_check with 1-2 simple shell commands in a Makefile target.

kdmccormick

As I've highlighted, existing issues with Paver have made it so that these checks are not working on master or your branch, so please just remove them entirely:

coverage-js 🪓
stylelint 🪓

Furthermore, the following checks do not rely on any complicated output parsing, so instead of being part of a Python script, please pull the underlying shell commands directly into their respective Makefile target:

check_keywords
pii_check

and, for the same reason, pull this into its own npm run command in package.json:

test-js

which will allow you to delete scripts/js_test.py.

That leaves just two commands in scripts/quality_test.py...

run_xsslint: This is a python function that uses a shell command to call a python script. That's bonkers (not your fault, it was that way before). The implementation in your PR, though, currently silently passes when there are xsslint violations. Instead of fixing it, let's simplify further. Please delete this function. Move the logic which compares violations against xsslint_thresholds.json logic into scripts/xsslint/xsslint/main.py. Finally, create a Makefile target which invokes scripts/xsslint/xss_linter.py directly.
run_eslint: This command currently works, both in the Paver implementation, and in your implementation. However, it is still more complicated than it needs to be. Please pull it into a new Python script called scripts/eslint.py. Keep it as simple as possible. Minimal variables, minimal functions, no constants. Do not generate any report files; just capture the output directly and parse the last line. Please also be sure to print() both the shell command and the command's output so that developers can understand the check when they look at the build log.

kdmccormick · 2024-10-29T18:40:46Z

scripts/js_test.py

+#     ("compare-branch=", "b", "Branch to compare against, defaults to origin/master"),
+# ], share_with=['coverage'])
+
+def diff_coverage():


Unfortunately, I have found that the JS coverage check does not actually validate anything, both on master and on this branch.

Here is a PR that removes a significant number of JS tests. You would expect that this would cause JS coverage to fail. However, both with the Paver checks and with your new Paver-free checks, it seems that coverage check is not actually doing anything. The operative log lines seems to be:

------------- Diff Coverage Diff: origin/master..HEAD, staged and unstaged changes ------------- No lines with coverage information in this diff. -------------

In my opinion, it is not worthwhile trying to turn JS coverage check back on. All the JS in edx-platform is legacy code. It is either deprecated or soon-to-be-deprecated. Please just remove all the JS coverage checks from this PR.

kdmccormick · 2024-10-29T18:51:23Z

scripts/js_test.py

+        cmd = [
+            "node",
+            "--max_old_space_size=4096",
+            "node_modules/.bin/karma",
+            "start",
+            self.test_conf_file,
+            "--single-run={}".format('false' if self.mode == 'dev' else 'true'),
+            "--capture-timeout=60000",
+            f"--junitreportpath={self.xunit_report}",
+            f"--browsers={Env.KARMA_BROWSER}",
+        ]


With diff_coverage removed from this script, the only thing that this module needs to do is run some shell commands that executes the javascript tests.

We do not need Python code to do that. Having the shell command wrapped in Python makes it more complicated and error-prone. Please replace this module with one or more node node_modules/.bin/karma ... command invocations in the Makefile. There are several assumptions you can make that should the Python code unnecessary:

We do not need to generate report files, for junit or for anything else.

We can assume that the browser is always Firefox

We can assume that the tests are being run from the repo root. No need to detect the repository root programmatically.

We can assume that SERVICE_VARIANT is None.

We can assume that we are running outside of Docker

Colored output printing is nice-to-have but not essential. I would rather save colored output printing for a follow-up PR if it meant finishing this PR faster.

@kdmccormick There is one thing if could guide me, When i run the command directly from Make file and in case it can not start Firefox browser the process does not kill and hangs on for an indefinite time.

Command in Make file:

@node --max_old_space_size=4096 node_modules/.bin/karma start cms/static/karma_cms.conf.js \ --single-run=false \ --capture-timeout=60000 \ --browsers=FirefoxNoUpdates

Console warning:

04 11 2024 14:18:37.622:INFO [launcher]: Starting browser Firefox
04 11 2024 14:19:37.633:WARN [launcher]: Firefox have not captured in 60000 ms, killing.
04 11 2024 14:19:39.680:WARN [launcher]: Firefox was not killed in 2000 ms, sending SIGKILL.
04 11 2024 14:19:41.688:WARN [launcher]: Firefox was not killed by SIGKILL in 2000 ms, continuing.

In the python code they are manually killing the process like

edx-platform/pavelib/utils/process.py

Line 16 in d0dbb8d

def kill_process(proc):

kdmccormick · 2024-10-29T18:57:38Z

scripts/quality_test.py

+    command = [
+        'node', 'node_modules/stylelint',
+        '*scss_files',
+        '--custom-formatter', 'stylelint-formatter-pretty/index.js'
+    ]


This command was silently passing because:

node_modules/stylelint is a directory, not a binary. The binary is at node_modules/.bin/stylelint. Running node on a directory does nothing.

*scss_files matches nothing. The correct pattern is **/*.scss

Regardless, though, the Paver version of this command doesn't work on master either. It has been silently failing with /bin/sh: 1: stylelint: not found for potentially several years now. There are now 24k+ sass style violations on master. Fixing all those violations would not be a good use of our time, especially since we intend to delete all the Sass code in edx-platform.

So, you can just remove all the stylelint checks.

kdmccormick · 2024-10-29T18:58:48Z

scripts/quality_test.py

-        ),
-        ignore_error=True
-    )
+    violations_limit = 4950


This threshold is way too high! I know it comes from the master branch. I don't know why it was set so high in the first place.

There are only 1213 eslint violations on master currently. Can you lower the threshold down to that?

pavelib/quality_test.py

scripts/generic-ci-tests.sh

Makefile

stylelint.config.js

scripts/quality_test/quality_test.py

salman2013 · 2024-11-04T14:34:35Z

scripts/js_test.py

+        cmd = [
+            "node",
+            "--max_old_space_size=4096",
+            "node_modules/.bin/karma",
+            "start",
+            self.test_conf_file,
+            "--single-run={}".format('false' if self.mode == 'dev' else 'true'),
+            "--capture-timeout=60000",
+            f"--junitreportpath={self.xunit_report}",
+            f"--browsers={Env.KARMA_BROWSER}",
+        ]


@kdmccormick There is one thing if could guide me, When i run the command directly from Make file and in case it can not start Firefox browser the process does not kill and hangs on for an indefinite time.

Command in Make file:

@node --max_old_space_size=4096 node_modules/.bin/karma start cms/static/karma_cms.conf.js \ --single-run=false \ --capture-timeout=60000 \ --browsers=FirefoxNoUpdates

Console warning:

04 11 2024 14:18:37.622:INFO [launcher]: Starting browser Firefox
04 11 2024 14:19:37.633:WARN [launcher]: Firefox have not captured in 60000 ms, killing.
04 11 2024 14:19:39.680:WARN [launcher]: Firefox was not killed in 2000 ms, sending SIGKILL.
04 11 2024 14:19:41.688:WARN [launcher]: Firefox was not killed by SIGKILL in 2000 ms, continuing.

In the python code they are manually killing the process like

edx-platform/pavelib/utils/process.py

Line 16 in d0dbb8d

def kill_process(proc):

salman2013 added 2 commits July 23, 2024 13:51

chore: replace paver quality tests

e140ef0

fix: remove un-used code

337f085

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 23, 2024

This comment was marked as outdated.

Sign in to view

salman2013 added 4 commits July 23, 2024 15:34

chore: fix some lint errors

145f2a4

fix: fix lint errors

06c9ec7

fix: fix method error

7ded572

fix: fix the lint errors

afa2f9f

salman2013 requested a review from kdmccormick July 23, 2024 11:20

salman2013 marked this pull request as ready for review July 23, 2024 11:21

salman2013 requested a review from feanil July 23, 2024 11:31

chore: remove paver tests for quality commands

5a6e526

salman2013 added 8 commits July 24, 2024 20:30

chore: remove paver tests for quality commands

b8557dc

chore: remove paver tests for quality commands

b551990

chore: remove pycodestyle violations and run the direct command

316ea9c

fix: fixing tests

fc71d83

fix: fix tests

424e220

fix: trying eslint command from make file

6b851cc

fix: fixing tests

17ae7a5

chore: add stylelint test in quality workflow

71a7c73

salman2013 closed this Jul 31, 2024

salman2013 reopened this Jul 31, 2024

salman2013 added 5 commits July 31, 2024 17:24

chore: add stylelint test in quality workflow

ec42adf

Merge remote-tracking branch 'upstream/master' into salman/remove-pav…

e4d2d0f

…er-commands

chore: fix tests

ad2e053

chore: fix tests

7f44491

chore: fix tests

e963d3f

salman2013 requested a review from kdmccormick August 27, 2024 07:18

salman2013 mentioned this pull request Aug 27, 2024

Replace Paver quality and js_test commands #34845

Open

kdmccormick requested changes Sep 4, 2024

View reviewed changes

salman2013 added 2 commits September 10, 2024 08:53

fix: fix the comments found in PR review

2f3169a

fix: fix the commands with check=True

75cf11c

salman2013 requested a review from kdmccormick September 12, 2024 10:47

Merge branch 'master' into salman/remove-paver-commands

c05f932

kdmccormick reviewed Oct 24, 2024

View reviewed changes

kdmccormick mentioned this pull request Oct 28, 2024

Side-by-side testing of Paver checks and New checks kdmccormick/edx-platform#26

Draft

kdmccormick requested changes Oct 28, 2024

View reviewed changes

kdmccormick requested changes Oct 29, 2024

View reviewed changes

salman2013 commented Nov 4, 2024

View reviewed changes

kdmccormick mentioned this pull request Nov 5, 2024

Re-enable pii_check linting #35775

Open

salman2013 added 4 commits November 11, 2024 17:07

fix: fix the comments, code improvements

2c68035

fix: testing changes in commands

f53eab8

fix: fix the code for stylelint removal

77476dd

fix: fix the pii_check command for django setting module

26d5153

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Paver quality and js_test commands #35159

Replace Paver quality and js_test commands #35159

salman2013 commented Jul 23, 2024 •

edited

Loading

This comment was marked as outdated.

salman2013 commented Jul 23, 2024 •

edited

Loading

kdmccormick commented Jul 23, 2024

kdmccormick commented Jul 24, 2024

salman2013 commented Aug 27, 2024

kdmccormick left a comment

salman2013 commented Sep 12, 2024

kdmccormick commented Oct 24, 2024

kdmccormick left a comment

kdmccormick Oct 24, 2024

kdmccormick Oct 24, 2024

kdmccormick commented Oct 25, 2024 •

edited

Loading

kdmccormick commented Oct 28, 2024

kdmccormick left a comment

kdmccormick Oct 28, 2024 •

edited

Loading

kdmccormick left a comment •

edited

Loading

kdmccormick Oct 29, 2024

kdmccormick Oct 29, 2024

salman2013 Nov 4, 2024

kdmccormick Oct 29, 2024

kdmccormick Oct 29, 2024

salman2013 Nov 4, 2024

Replace Paver quality and js_test commands #35159

Are you sure you want to change the base?

Replace Paver quality and js_test commands #35159

Conversation

salman2013 commented Jul 23, 2024 • edited Loading

This comment was marked as outdated.

salman2013 commented Jul 23, 2024 • edited Loading

kdmccormick commented Jul 23, 2024

kdmccormick commented Jul 24, 2024

salman2013 commented Aug 27, 2024

kdmccormick left a comment

Choose a reason for hiding this comment

salman2013 commented Sep 12, 2024

kdmccormick commented Oct 24, 2024

kdmccormick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick commented Oct 25, 2024 • edited Loading

kdmccormick commented Oct 28, 2024

kdmccormick left a comment

Choose a reason for hiding this comment

kdmccormick Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

kdmccormick left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salman2013 commented Jul 23, 2024 •

edited

Loading

salman2013 commented Jul 23, 2024 •

edited

Loading

kdmccormick commented Oct 25, 2024 •

edited

Loading

kdmccormick Oct 28, 2024 •

edited

Loading

kdmccormick left a comment •

edited

Loading